feat: fix tranche key ordering by sotnikov-s · Pull Request #1081 · neutron-org/neutron

sotnikov-s · 2026-05-07T15:03:46Z

this PR:

resolves issue about a newer expiring order can be filled before an older one at the same price because the DEX compares tranche IDs as plain text. #1072 by using Uint64ToSortableString func on tranche key indices: tk-100500 becomes tk-Uint64ToSortableString(100500). this ensures proper lexicographic ordering
adds dex storage migration for limit order tranches, inactive limit order tranches, limit order tranche users, and limit order expirations. affected storage leafs are the ones that contain "tk-" prefix.

how to verify this code:

https://www.notion.so/hadron/dex-tk-0-migration-verification-35f85d6b9b1080e28044f4d4f793a285?source=copy_link#36e85d6b9b1080a996c4ef23fe9c7832

… tranche key

…ranche-key-ordering

jcompagni10 · 2026-05-20T18:39:51Z

+	return nil
+}
+
+func reconstructLoExpirations(ctx sdk.Context, k dexkeeper.Keeper) error {


Why does this need to be a separate process? Why can't we just iterate through all limit order tranches and then check the tranche directly if it is expiring?

I found explicit per-storage-branch functions easier to understand and safer: we iterate over the whole bunch of leafs at a time and leave no room for a mistake/misunderstanding. I could do as you suggested but in my opinion it would be a more complex migration code with no perceptible practical benefits like speed or efficiency

jcompagni10 · 2026-05-20T20:58:22Z

+
+// NewTrancheKey returns a new tranche key based on the tranche index.
+func NewTrancheKey(trancheIdx uint64) string {
+	return fmt.Sprintf("tk-%020d", trancheIdx)


Probably not a big deal, but why not use Uint64ToSortableString? It's a bit more space-efficient. Also, from a UX perspective, trying to count/type a bunch of leading zeros is a PITA.

no strong point against this approach, I also considered it, but preferred the zero-padded format for easier testing and debugging. re UX perspective: 1) I don't think anyone (but me and you in scope of this PR haha) would ever count the zeroes since it's easier and more useful to count the non-zero digits 2) if someone ever needs to get to know their tranche index, would it be easier for them to parse the base36 number prefixed with length?

space efficiency is a good point. you suggest to use tk-Uint64ToSortableString(trancheIdx) instead of tk-020%d, right? do you guys think it worths refactoring?

agree Uint64ToSortableString function, I would even get rid of the tk prefix - it doesnt seem to carry any useful information

moved from zero padded tranche keys to Uint64ToSortableString usage. didn't remove the tk- prefix since it ruined proper lexicographical ordering: there are obsolete keys build with height + block gas usage that also have that base36 format build with Uint64ToSortableString and they all start with 5 as well as the tk-plain-decimal keys do. e.g. there is a tranche user record with tranche key 5ak8d741gqz, and after migration a typical tranche key reconstructed from plain decimal looks like 52ojj9. the former key is older than the latter whereas lexicographic sorting puts the latter one before the former, and this is incorrect and it should be mitigated: older tranche keys must be put before newer ones in sorting/iterating.

also addressed this comment:

It seems to me that this kind of migration should be performed in a numbered module migration rather than in an upgrade handler, just like any storage migration required for the correct operation of the new version of the code.

read about the results and dex module exported states here https://www.notion.so/hadron/dex-tk-0-migration-verification-35f85d6b9b1080e28044f4d4f793a285?source=copy_link#36e85d6b9b1080a996c4ef23fe9c7832

swelf19 · 2026-05-26T12:36:18Z

It seems to me that this kind of migration should be performed in a numbered module migration rather than in an upgrade handler, just like any storage migration required for the correct operation of the new version of the code.

…ge migration handler

jcompagni10 · 2026-05-29T18:20:20Z

This all looks good. Maybe add a test to confirm that the 3 types of tranches keys (sortableString, tk-[n], and tk-[sortable-string] all get stored in the correct order. I think it is correct. But a test would be nice.

swelf19

Am I understanding correctly that these are breaking changes? If an application stored the tk when it was created, it will unexpectedly receive an error when trying to work with the old tk later?
Does it make sense to add a note or comment about this in the code and mention it in a future release?

TrancheUserKey has a back-link to tk. It seems those need to be updated as well?

…actor it

sotnikov-s · 2026-06-01T17:22:24Z

This all looks good. Maybe add a test to confirm that the 3 types of tranches keys (sortableString, tk-[n], and tk-[sortable-string] all get stored in the correct order. I think it is correct. But a test would be nice.

done here: 9c83710

sotnikov-s · 2026-06-01T17:24:57Z

TrancheUserKey has a back-link to tk. It seems those need to be updated as well?

doesn't this part of the migration address this?

neutron/x/dex/migrations/v9/store.go

Lines 195 to 229 in 9c83710

    
           func reconstructLoTrancheUserLists(ctx sdk.Context, k dexKeeper) error { 
        
           	allUsers := k.GetAllLimitOrderTrancheUser(ctx) // there are only 300-ish entries, so getting all is fine 
        
           	usersToRemove := make([]dextypes.LimitOrderTrancheUser, 0) 
        
           	usersToUpdate := make([]dextypes.LimitOrderTrancheUser, 0) 
        
           	for _, user := range allUsers { 
        
           		if !strings.HasPrefix(user.TrancheKey, "tk-") { 
        
           			continue 
        
           		} 
        
           		usersToRemove = append(usersToRemove, *user) 
        
           		trancheIdxStr := strings.TrimPrefix(user.TrancheKey, "tk-") 
        
           		trancheIdx, err := strconv.ParseUint(trancheIdxStr, 10, 64) 
        
           		if err != nil { 
        
           			return fmt.Errorf("failed to parse tranche idx %s: %w", trancheIdxStr, err) 
        
           		} 
        
           		user.TrancheKey = dextypes.NewTrancheKey(trancheIdx) 
        
           		usersToUpdate = append(usersToUpdate, *user) 
        
           	} 
        
           	if len(usersToRemove) != len(usersToUpdate) { 
        
           		return fmt.Errorf("mismatch in LO tranche user keys to remove and update counts: %d != %d", len(usersToRemove), len(usersToUpdate)) 
        
           	} 
        
           	for _, user := range usersToRemove { 
        
           		k.RemoveLimitOrderTrancheUser(ctx, &user) 
        
           	} 
        
           	for _, user := range usersToUpdate { 
        
           		k.SetLimitOrderTrancheUser(ctx, &user) 
        
           	} 
        
           	ctx.Logger().Info("LO tranche user keys reconstructed", "count", len(usersToUpdate)) 
        
           	return nil 
        
           }

sotnikov-s · 2026-06-01T17:38:34Z

Am I understanding correctly that these are breaking changes? If an application stored the tk when it was created, it will unexpectedly receive an error when trying to work with the old tk later? Does it make sense to add a note or comment about this in the code and mention it in a future release?

I guess so: some msgs and queries have tranche key as a parameter, so if e.g. a smart contract memorised a plain decimal tranche key e.g. tk-100 and it then tries to do something with that tranche using that stored key, it won't work because the tranche key will become tk-22S after migration. don't think it makes sense adding a comment in a code but a note in a future release makes sense to me

sotnikov-s · 2026-06-02T06:56:57Z

https://github.com/neutron-org/neutron-tests/actions/runs/26770936506

sotnikov-s added 8 commits May 7, 2026 18:01

improve tranche key creation helpers, add up to 20 trailing zeroes to…

017b256

… tranche key

add inactive LO tranche iterator getter to dex keeper

6562e91

draft version of tk-N -> tk-000...N upgrade handler

aa20deb

add nextupgrade to the list of upgrades

4c166c9

add tk migration logging and sanity checks

f56294f

simpliofy reconstructLoTrancheUserLists

bd85f81

add storage migration for LO expirations

539a370

Merge branch 'main' of github.com:neutron-org/neutron into feat/fix-t…

558a067

…ranche-key-ordering

sotnikov-s marked this pull request as ready for review May 19, 2026 14:47

fix linter

b08bebe

jcompagni10 previously approved these changes May 20, 2026

View reviewed changes

sotnikov-s added 3 commits May 27, 2026 12:31

move NewLimitOrderExpiration from dex keeper to types

060338a

move dex tranche keys migration from app upgrade handler to dex stora…

f792916

…ge migration handler

tranche keys as Uint64ToSortableString instead of tk-%020d

86f04ef

sotnikov-s dismissed jcompagni10’s stale review via 86f04ef May 27, 2026 12:01

sotnikov-s marked this pull request as draft May 27, 2026 16:04

sotnikov-s added 2 commits May 28, 2026 20:18

add tk- prefix to new tranche key format for correct ordering

4b6367d

add better logging to dex storage migration to v9

66c4510

jcompagni10 previously approved these changes May 29, 2026

View reviewed changes

swelf19 previously approved these changes Jun 1, 2026

View reviewed changes

move dex tk reconstruction test to v9 store migration package and ref…

9c83710

…actor it

sotnikov-s dismissed stale reviews from swelf19 and jcompagni10 via 9c83710 June 1, 2026 17:21

sotnikov-s requested review from jcompagni10 and swelf19 June 2, 2026 06:56

sotnikov-s marked this pull request as ready for review June 2, 2026 06:56

swelf19 approved these changes Jun 2, 2026

View reviewed changes

zavgorodnii force-pushed the main branch from 9582884 to 9195772 Compare July 14, 2026 09:57

Uh oh!

Conversation

sotnikov-s commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

this PR:

how to verify this code:

Uh oh!

jcompagni10 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

sotnikov-s May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcompagni10 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

sotnikov-s May 21, 2026

Choose a reason for hiding this comment

Uh oh!

sotnikov-s May 21, 2026

Choose a reason for hiding this comment

Uh oh!

swelf19 May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sotnikov-s May 29, 2026

Choose a reason for hiding this comment

Uh oh!

swelf19 commented May 26, 2026

Uh oh!

jcompagni10 commented May 29, 2026

Uh oh!

swelf19 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sotnikov-s commented Jun 1, 2026

Uh oh!

sotnikov-s commented Jun 1, 2026

Uh oh!

sotnikov-s commented Jun 1, 2026

Uh oh!

sotnikov-s commented Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sotnikov-s commented May 7, 2026 •

edited

Loading

sotnikov-s May 21, 2026 •

edited

Loading

swelf19 May 26, 2026 •

edited

Loading

swelf19 left a comment •

edited

Loading